Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Update test harness with metadata assertions #1467 #289

Merged
merged 5 commits into from
Jan 27, 2025

Conversation

chrfwow
Copy link
Contributor

@chrfwow chrfwow commented Jan 13, 2025

This PR

Adds gherkin test to verify that flag evaluations provide metadata

Related Issues

Part of #1467 (open-feature/flagd#1467)

Follow-up Tasks

Implement steps of the gherkin file in the repositories, and add test data according to the issue

@chrfwow chrfwow requested a review from toddbaert as a code owner January 13, 2025 12:19
@chrfwow chrfwow force-pushed the Update-test-harness-#1467 branch from feec001 to ff27af0 Compare January 13, 2025 12:21
@toddbaert
Copy link
Member

toddbaert commented Jan 13, 2025

@aepfli do you think it makes more sense to simply add metadata to the existing gherkin suite? We already have error flags there - we could add metadata to some of the existing flags and error flags, and them update the in-memory providers to support this.

@aepfli
Copy link
Member

aepfli commented Jan 14, 2025

@aepfli do you think it makes more sense to simply add metadata to the existing gherkin suite? We already have error flags there - we could add metadata to some of the existing flags and error flags, and them update the in-memory providers to support this.

I am not sure if integration into the existing suite is suitable. We could enhance the existing ones, with a description of what the purpose of those flags is. But generally i prefer a flagname which already tells me what i can expect from this flag.

This also brings me to the next topic. As we are currently generating the data in each sdk on our own, it might be worth to add a json here with the testdata, which we load into the inmemory provider, wdt?

@chrfwow chrfwow requested a review from guidobrei January 15, 2025 09:47
@toddbaert
Copy link
Member

This also brings me to the next topic. As we are currently generating the data in each sdk on our own, it might be worth to add a json here with the testdata, which we load into the inmemory provider, wdt?

Most in-memory providers can't actually load JSON, so I'm not sure how valuable that would be.

@aepfli
Copy link
Member

aepfli commented Jan 15, 2025

This also brings me to the next topic. As we are currently generating the data in each sdk on our own, it might be worth to add a json here with the testdata, which we load into the inmemory provider, wdt?

Most in-memory providers can't actually load JSON, so I'm not sure how valuable that would be.

i would not load them automagically, but within the tests, we can load the json files, and provide the data as parameter to the inmemory provider. We would centralize the test flag data, and with new gherkin files, which only add new data for experimentation everything should work ootb.

data = loadJsonData(jsonfile)
provider = InMemoryProvider(data) // simple prosa

Co-authored-by: Simon Schrottner <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
@toddbaert
Copy link
Member

This also brings me to the next topic. As we are currently generating the data in each sdk on our own, it might be worth to add a json here with the testdata, which we load into the inmemory provider, wdt?

Most in-memory providers can't actually load JSON, so I'm not sure how valuable that would be.

i would not load them automagically, but within the tests, we can load the json files, and provide the data as parameter to the inmemory provider. We would centralize the test flag data, and with new gherkin files, which only add new data for experimentation everything should work ootb.

data = loadJsonData(jsonfile)
provider = InMemoryProvider(data) // simple prosa

Oh, so all implementations would write a simple function loadJsonData or similar which would do the conversion? I guess that's not a bad idea since it might be fairly future-proof when new things are added.

@toddbaert toddbaert changed the title feat: Update test harness (add assertions) #1467 feat: Update test harness with metadata assertions #1467 Jan 15, 2025
@aepfli
Copy link
Member

aepfli commented Jan 15, 2025

Oh, so all implementations would write a simple function loadJsonData or similar which would do the conversion? I guess that's not a bad idea since it might be fairly future-proof when new things are added.

excactly, and this way we ensure, everyone gets the same data afterall

@toddbaert
Copy link
Member

Merging this at just 2 green checks because it's just some test data and doesn't represent any fundamental new features or changes.

@toddbaert toddbaert merged commit 5b07065 into open-feature:main Jan 27, 2025
7 checks passed
@chrfwow chrfwow deleted the Update-test-harness-#1467 branch January 27, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants